Document BuildJsScope model and split tracking scripts - #601
Document BuildJsScope model and split tracking scripts#601promptless-for-oss wants to merge 5 commits into
Conversation
Documents the JS build scope model from mautic/mautic#16660: the BuildJsScope enum, BuildJsEvent::appendJsForScope()/acceptsScope(), the /mautic-essential.js and /mautic-tracking.js endpoints, client-side runtime globals, and the backward-compatibility impact on legacy appendJs() subscribers.
patrykgruszka
left a comment
patrykgruszka
left a comment
There was a problem hiding this comment.
The scoped extension API and appendJs() migration guidance are useful additions. Before merging, the essential script’s capabilities should be narrowed to match the tested DWC fallback and form behavior, RUNTIME should not be presented as interchangeable with ESSENTIAL, and the readiness example should clearly state that mauticEssentialReady depends on the shipped consent-managed loader rather than the endpoint itself. With those reductions, this documentation will provide useful and accurate plugin guidance.
…lt, clarify mauticEssentialReady loader dependency
|
Thanks for the detailed review, @patrykgruszka — all three points are addressed and pushed to PR #601:
All claims were re-verified against the PR #16660 source, and no new Vale issues were introduced. Ready for another look. |
patrykgruszka
left a comment
patrykgruszka
left a comment
There was a problem hiding this comment.
Good to go from my perspective 👍
adiati98
left a comment
adiati98
left a comment
There was a problem hiding this comment.
@promptless-for-oss please address the suggestions.
|
Thanks @adiati98 — on it. We will switch the flagged sentences to active voice ('is treated', 'It's excluded'), replace the parenthetical scope list with a hyphenated form where it is not inline code, and address the Vale FeatureList warning on 'Reports'. We will push an update to this PR shortly. |
…ureList
- Rewrite the appendJs note in active voice ('is treated'/'It's excluded')
- Replace parentheses around the inline-code scopes array with a spaced-hyphen aside
- Reword the verb 'reports' to 'exposes' to clear the Mautic.FeatureList Vale warning
|
@adiati98 thanks for the review — all three points are addressed and pushed:
Vale is clean apart from one pre-existing finding on an unchanged line (line 64, One note: we tried to re-request your review programmatically but GitHub returned a 404 (the service account can't add a reviewer on this cross-fork PR). Would you mind re-triggering your review manually when you have a moment? |
|
@promptless-for-oss can you please investigate why read the docs (RTD) has pending check? I've checked the RTD's dahsboard and the build for this PR succeeded. |
|
Thanks @adiati98 — we are looking into why the Read the Docs check still shows as pending here even though the RTD build itself succeeded. We will follow up on this thread with what we find. |
|
@adiati98 we looked into the pending Read the Docs check. The RTD build itself did succeed — but its status was recorded against an earlier commit in the PR, not the current head commit (the merge commit "Merge branch '7.2' into promptless/pr-16660-mtc-script-split-7.2"). GitHub reads the check state from the head commit, which has no RTD status attached, so the check appears stuck on 'pending' even though the build passed on the RTD dashboard. This is a known quirk of the RTD ↔ GitHub integration when a PR is updated with a merge/sync — not a problem with the docs or the build itself. To clear it, any of these will make RTD post its status to the current head commit:
We held off doing this ourselves since it is an outward-facing action on a PR under your active review — happy to push a trivial commit to re-sync it if you would like us to; just let us know. |
|
@promptless-for-oss I've triggered the rebuild from the RTD dashboard and close and reopen the PR with no luck. Please try to push an empty commit and see if it can trigger it. |
|
Thanks @adiati98 — will do. We are pushing an empty commit to the branch to force RTD to re-post its status against the current head, and will report back here whether the check clears. |
Empty commit to prompt Read the Docs to re-post its build status on the current PR head, as requested by @adiati98 (RTD dashboard rebuild and close/reopen did not refresh the pending status).
|
@adiati98 update on the pending Read the Docs check: we pushed the empty commit as you asked. It landed correctly as the PR head and did re-trigger your GitHub Actions 'Linting' checks (prose and build both re-ran) — so the push itself works. However, Read the Docs still posted no commit status at all: the check stayed 'pending' with nothing reported across about 10 minutes of monitoring. Because the commit triggered GitHub Actions but not RTD, this looks like a Read the Docs integration issue on the project side rather than something another commit can fix — further empty commits will just re-run CI, not RTD. Since this is on the RTD project configuration (which we do not have access to), could you check, in the Read the Docs project settings:
Re-syncing or re-adding the webhook there should let RTD post its status to the current commit and clear the pending check. Happy to help further from our side once that is in place — just let us know. |
3 participants
Updates the MauticJS tracking script developer page (
docs/mauticjs_api/tracking_script.rst) to document the JS build scope model introduced in mautic/mautic#16660 ("Split mtc.js into essential and tracking scripts", MTC-11100).New developer-facing surface documented:
Mautic\CoreBundle\Event\BuildJsScopeenum (RUNTIME,ESSENTIAL,TRACKING).BuildJsEvent::appendJsForScope($js, BuildJsScope $scope, $section = '')andacceptsScope(BuildJsScope $scope): bool, with plugin-style PHP subscriber examples.appendJs()now delegates toappendJsForScope()withBuildJsScope::TRACKING, plus a backward-compatibility warning (with argument-position migration guidance) thatappendJs()-only subscribers are excluded from/mautic-essential.js./mautic-essential.js,/mautic-tracking.js,/mtc.js) and their scope mappings.MauticJS.runtimeReady,trackingEnabled,requestWithCredentials) and themauticEssentialReadyevent, with a safe dual-path readiness guard example.Source PR: mautic/mautic#16660
Review response (@patrykgruszka)
Thanks for the review. All three comments applied:
ESSENTIALscope bullet and the/mautic-essential.jsendpoint-table row no longer say "Dynamic Content rendering and Form injection." They now describe it as preserving existing Dynamic Content fallback content without making a new request, and initializing Forms already embedded in that fallback content — matchingMauticScriptSplitCest.php(essential-only mode keeps the DWC fallback without a DWC request) andPublicControllerFunctionalTest.php(standalone forms are independent of all tracking scripts).RUNTIMEno longer offered as a plugin target — applied. Removed the "orBuildJsScope::RUNTIME" alternative from the migration warning; plugin pre-consent code is now directed toappendJsForScope()withBuildJsScope::ESSENTIAL(with optionalacceptsScope()gating).RUNTIMEremains described only as the core bootstrap the other scopes depend on, consistent with its single use inCoreBundle/EventListener/BuildJsSubscriber.php.mauticEssentialReadydependency on the shipped loader made explicit — applied. A dedicated paragraph after the readiness example now states the event is dispatched by the shipped consent-managed loader snippet, not by/mautic-essential.jsitself, and tells custom-loader users to run their code from their own script tag'sload/onloadcallback onceMauticJS.runtimeReady === true, or dispatch an equivalent event themselves.All edits applied verbatim to intent; the only deviations from the literal comment wording were style-driven (dropped the
(DWC)abbreviation in favor of "Dynamic Content" per the Mautic ValeFeatureListrule, replaced an "and/or" with an explicit required-action/optional-gate split, and moved the custom-loader caveat into its own paragraph after the code block for findability). No new Vale errors introduced; the one pre-existing baseline finding on unchanged content remains.Review response (@adiati98)
Thanks for the review. All three comments applied:
appendJs().. note::no longer uses the passive "is treated as" / "It's excluded from." It now reads: "so Mautic treats code appended this way asTRACKINGscope and includes it only in tracking-enabled builds - including/mtc.jsand/mautic-tracking.js- but excludes it from/mautic-essential.js."[BuildJsScope::RUNTIME, BuildJsScope::ESSENTIAL, BuildJsScope::TRACKING]is now set off by a spaced-hyphen aside rather than wrapped in prose parentheses.Mautic.FeatureListVale warning on "reports" — applied with deviation. This was a false positive on the verb "to report," not the Report feature, so capitalizing to "Reports" would have been incorrect. Instead the verb was reworded: "theBuildJsEventexposes which scopes the current build accepts." This clears the warning without misrepresenting the sentence.No new Vale errors introduced; the one pre-existing baseline finding on unchanged content remains.
This documentation update was prepared by Promptless.